WIP: initial implementation of cursors#306
WIP: initial implementation of cursors#306geoffreylitt wants to merge 7 commits intoautomerge:mainfrom geoffreylitt:cursors
Conversation
frontend/index.js
Outdated
| * - consider returning the closest character if deleted | ||
| * - consider optimizing the linear search | ||
| */ | ||
| function findCursorIndex (cursor, doc, findClosest) { |
There was a problem hiding this comment.
I think it's odd that this function requires doc as a separate argument, but I couldn't find any way to navigate from a cursor object to the root doc.
|
Thanks @geoffreylitt, this is a good start! I pushed a few commits to change some details. In particular, I want to avoid the frontend calling directly into the backend, since that will cause them to become much more closely coupled than they should be. I moved the the We will need to make the equivalent change in the Rust implementation to maintain compatibility between the two implementations. This should not be hard. I will call out that this approach to cursors is incompatible with running frontend and backend on separate threads, since it relies on synchronously calling into the backend. I think that's probably an okay trade-off to make since most Automerge users probably aren't using multiple threads, and having this direct call into the backend is a lot simpler than updating the frontend-backend protocol. But weakening our support for multiple threads is something we should discuss, and make an explicit decision about. |
|
This is great work, I'm glad someone is taking on the cursor challenge 🙂 I'm also concerned about the explicit coupling to the backend. If we rule out the use of different threads for the frontend/backend protocol (and we would be ruling it out in Rust, the Backend and Frontend components have not been designed to be threadsafe and would require a significant rewrite to enable it) then we lose a lot of advantages the design gives us. I'm currently working on performance for the Rust codebase and aiming to make any change that happens on the UI thread fit within a 16ms budget in order to make 60FPS UI possible, removing multithreading makes this extremely difficult. Is it possible to achieve this same interface by adding an index to the frontend for the element ID <-> list index rather than asking the backend for that information? |
| */ | ||
| getCursorAt (index) { | ||
| return { | ||
| // todo: are there any points in the lifecycle where the Text object doesn't have an ID? |
There was a problem hiding this comment.
@ept I see some other places that handle the case where the object ID is not set.
Would it be sensible to add a similar check here and forbid getting a cursor if there's not an object ID? I don't yet understand when/how that case occurs.
|
@ept thanks, all those updates make sense. re: frontend/backend split -- as an alternative to @alexjg 's suggestion of maintaining more state in the frontend, what if I guess one problem might be that until now the protocol has entirely consisted of change requests going one way and patches going the other, and this would be adding a new kind of message that would need to be handled? |
An attempt at a simple initial implementation of cursors, as discussed in #239.
Automerge.Frontend.findCursorIndexwhich operates on that object, because that was easiest to implement. Probably would provide a nicer API to switch it to a custom datatype?